Vtr task and test script python rewrite #1509

shadtorrie · 2020-08-27T18:01:48Z

run_reg_test, run_vtr_task, parse_vtr_task and parse_vtr_flow have all been converted to python

Description

Using the old python rewrite branch as a starting place, I converted run_reg_test.pl, run_vtr_task.pl, parse_vtr_task.pl and parse_vtr_flow.pl to run_reg_test.py, run_vtr_task.py, parse_vtr_task.py and parse_vtr_flow.py.
These should be direct replacements of the perl counterpart.
Be aware that I placed the parse_vtr_task and parse_vtr_flow within the python library created by the run_vtr_flow pull request.

Related Issue

The issue this partially fixes

Motivation and Context

We want to be able to run the vtr test suite without requiring perl to be installed.

How Has This Been Tested?

All regression tests pass by using run_reg_test.
I tested the scripts individually and together, ensuring all parameters are enabled and work.

Types of changes

Bug fix (change which fixes an issue)
New feature (change which adds functionality): run the python script counterpart instead of perl
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation
I have updated the documentation accordingly
I have added tests to cover my changes
All new and existing tests passed: This is contingent on the golden results failure being fixed by this pull request.

litghost · 2020-08-27T20:49:05Z

It doesn't look like the kokoro CI scripts were cut over to use the new python root script, see here: https://github.com/verilog-to-routing/vtr-verilog-to-routing/blob/master/.github/kokoro/steps/vtr-test.sh#L48

shadtorrie · 2020-08-27T22:08:19Z

@litghost I have fixed that instance and others of the older perl scripts.

shadtorrie · 2020-08-27T22:41:26Z

Kokoro tests will fail right now, as pointed out by @litghost here. This will be fixed once the requirements pull request is merged

shadtorrie · 2020-08-27T22:42:26Z

And one Qor test will fail on vtr_reg_Strong until this pull request is merged

HackerFoo

Please try to abstract out anything that might be useful to change, such as paths, and document file formats that are parsed or generated.

Parsing could be made more flexible by not expecting fixed parameter names.

HackerFoo · 2020-08-27T23:07:20Z

vtr_flow/parse/parse_config/common/abc.txt


 #ABC Run-time Metrics
 abc_synth_time;abc0.out;elapse: .* seconds, total: (.*) seconds
 abc_cec_time;abc.cec.out;elapse: .* seconds, total: (.*) seconds
-abc_sec_time;abc.sec.out;elapse: .* seconds, total: (.*) seconds
+abc_sec_time;abc.lec.out;elapse: .* seconds, total: (.*) seconds


What does this change do?

This changes which file it looks for this parameter. I believe by the request of Vaughn the file name was changed from abc.sec.out to abc.lec.out.

Nope, I didn't ask for this (don't recall anything about it). Is this the logical equivalence check runtime (and hence maybe lec is a better name than sec, which would seem to imply sequential equivalence check)?

Oh sorry, it was Kevin, #1429 (comment)

HackerFoo · 2020-08-27T23:09:27Z

vtr_flow/scripts/parse_vtr_task.pl

 	my $task_name = shift;
 	(my $task_path = $task_name) =~ s/\s+$//;
-
+	


Extra whitespace

This has been fixed

HackerFoo · 2020-08-27T23:13:31Z

run_reg_test.py

+            "odin_reg_micro",
+            "vtr_reg_valgrind_small",
+        ],
+        help="Regression tests to be run",


Should these tests be in a config file, or inferred from the directory structure?

This has been fixed

HackerFoo · 2020-08-27T23:15:39Z

run_reg_test.py

+                        row += [(values[2] % float(info[values[0]])) + values[1]]
+            table.add_row(row)
+        print(table)
+    return 0


This function has a lot of numbers without any explanation. Please name them.

This has been fixed

HackerFoo · 2020-08-27T23:17:26Z

run_reg_test.py

+    vtr_task_list_files = []
+    if reg_test.startswith("vtr"):
+        task_list_filepath = str(
+            Path(find_vtr_root())


Can this path be an argument or environment variable? At the least, please put arbitrary constants (such as a location which could change) at the top of the file with a description.

An environment variable is used in the function find_vtr_root() I updated this to use it more efficiently. Let me know if there is anything else I should do with this.

HackerFoo · 2020-08-27T23:29:35Z

vtr_flow/scripts/python_libs/vtr/parse_vtr_task.py

+                with redirect_stdout(parse_file):
+                    parse_vtr_flow(job.parse_command())
+        if job.second_parse_command():
+            parse_filepath = str(PurePath(work_dir) / "parse_results_2.txt")


Please move arbitrary file names to top of file.

This has been fixed

HackerFoo · 2020-08-27T23:30:37Z

vtr_flow/scripts/python_libs/vtr/parse_vtr_task.py

+    # We do not worry about non-pass_requriements elements being different or missing
+    pass_req_filepath = str(
+        PurePath(find_vtr_root())
+        / "vtr_flow"


Avoid hard-coded paths

This has been fixed

HackerFoo · 2020-08-27T23:33:19Z

vtr_flow/scripts/python_libs/vtr/task.py

+    )
+
+    # Keys that are repeated to create lists
+    repeated_keys = set(["circuit_list_add", "arch_list_add", "script_params_list_add"])


The pattern is that lists are built from *_list_add parameters.

This has been fixed

HackerFoo · 2020-08-27T23:43:20Z

vtr_flow/scripts/run_vtr_task.py

+    Path(work_dir).mkdir(parents=True)
+    run_script_file = Path(work_dir) / "vtr_flow.sh"
+    with open(run_script_file, "w+") as out_file:
+        print("#!/usr/bin/env bash", file=out_file)


Instead of embedding this, could this be a template that is loaded?
At the very least, could this be joined into one string and one call to format?

I created a template in the newest commit.

HackerFoo · 2020-08-27T23:44:15Z

vtr_flow/scripts/run_vtr_task.py

+    """ format the number of seconds given as a human readable value """
+    if seconds < 60:
+        return "%.0f seconds" % seconds
+    if seconds < 3600:


Why not "60 * 60" ?

This has been fixed

vaughnbetz · 2020-09-04T22:06:59Z

Travis CI failed on the Odin-II Micro tests, with what looks like test script issue:

 Ok . . . . . . . . . . . . . . . . . mults_auto_none/twobits_arithmetic_multiply/k6_N10_40nm
ran test in: 0:25:37.915
no run failure!
PASSED test 'odin_reg_micro'
Traceback (most recent call last):
  File "./run_reg_test.py", line 361, in <module>
    vtr_command_main(sys.argv[1:])
  File "./run_reg_test.py", line 184, in vtr_command_main
    vtr_task_list_files = collect_task_list(reg_test)
  File "./run_reg_test.py", line 325, in collect_task_list
    raise IOError("Test does not exist: {}".format(reg_test))
OSError: Test does not exist: odin_reg_micro
The command "./run_reg_test.py odin_reg_micro -show_failures -j2" exited with 1.

@shadtorrie can you take a look?

shadtorrie · 2020-09-06T04:23:06Z

This has been fixed.

vaughnbetz · 2020-09-07T18:45:54Z

Not sure why kokoro hasn't started yet; trying to force a restart.

vaughnbetz · 2020-09-07T20:34:10Z

Looks like there is an import failure that's killing the VtR Strong Tests:

++ echo ========================================
========================================
++ echo 'Running Tests'
Running Tests
++ echo ========================================
========================================
++ export VPR_NUM_WORKERS=1
++ VPR_NUM_WORKERS=1
++ ./run_reg_test.py vtr_reg_strong -show_failures -j8
Traceback (most recent call last):
  File "./run_reg_test.py", line 11, in <module>
    from prettytable import PrettyTable
ImportError: No module named 'prettytable'

@shadtorrie can you take a look?

shadtorrie · 2020-09-07T20:35:45Z

@vaughnbetz Yes this is fixed in my add python requirements pull request, once that is merged then this will not occur.

HackerFoo · 2020-09-08T17:18:10Z

run_reg_test.py

+    print("=" * 121)
+    print("\t" * 6, end="")
+    print("{} QoR Results".format(reg_test))
+    print("=" * 121)


Should the width be parameterized? Or at least add a function to print a header (lines 229-232.)

This has been fixed

HackerFoo · 2020-09-08T17:20:44Z

run_reg_test.py

+    """Call 'run_vtr_task' with all the required arguments in the command"""
+    print("Running {}".format(args.reg_test[0]))
+    print(
+        "-------------------------------------------------------------------------------"


Here's another divider with a different width. It would be nice to either have these be the same width, or add a function to print some text with an underline.

This has been fixed.

jgoeders · 2020-09-11T19:15:49Z

This is waiting on #1510

jgoeders · 2020-09-12T23:30:19Z

#1510 is now merged. Just waiting on CI then this should be good to go.

jgoeders · 2020-09-15T03:20:08Z

@litghost @mithro

The VTR Strong is running fine on kokoro, but VTR Strong Sanitized is timing out. Any ideas? How do I check if this his happening on master? (I know how to check Travis master but not Kokoro).

vaughnbetz · 2020-09-15T03:34:29Z

vtr strong sanitized is passing on other recent PRs, so I don't think the master is broken. The last ruptime indicates it takes a little more than 1h 21 m (vs. 2 hour timeout). See https://source.cloud.google.com/results/invocations/07b4a0f7-69ef-43ca-aef1-38266d1fc726/targets/foss-fpga-tools%2Fverilog-to-routing%2Fupstream%2Fpresubmit%2Fstrong_sanitized/log from PR #1541

You could trigger a CI on the master if you want to check by making a trivial change PR. May be more elegant ways than that, but that's the easiest.

I suggest running vtr strong sanitized locally with the new infrastructure and seeing if it passes, and if so, how long it takes. If something is hanging hopefully it's more clear from what hangs what the problem is.

Note that there are some expected vpr failures (checking bad input arguments, routing failures etc.) which is why the output in the link above has some detailed output and OK* values. (OK* means we expected a CAD flow failure, and we got one, so OK).

litghost · 2020-09-15T16:44:36Z

@litghost @mithro

The VTR Strong is running fine on kokoro, but VTR Strong Sanitized is timing out. Any ideas? How do I check if this his happening on master? (I know how to check Travis master but not Kokoro).

Any chance you broke the "-j" flag? That might explain the jump in runtime?

jgoeders · 2020-09-16T23:40:35Z

Looks to be fixed now. I'll see how the nightly goes, but the sanitized is working now.

The issue was related to how the multiprocessing.Pool tasks were queued up. The original method was:

pool.starmap(run_vtr_flow_process, queued_procs)

When I ran locally and watched it with top, I noticed that it would kick off 8 processes, but after a while it seemed like it was just one process running the last handful of jobs sequentially. It seemed like perhaps the work items were being split up ahead of time among the 8 worker threads, rather than the workers taking one item at a time as they completed. I think this was triggering the timeout check on kokoro. I tried starmap_async as well, but this didn't change anything.

I changed the code to

for proc in queued_procs:
    pool.apply_async(run_vtr_flow_process, proc)

and now it is working as expected. It's hard to find much information about this online, and from what I've read these two approaches should be doing the same thing, but in reality they weren't. Not sure if this is a Python bug in starmap, or some undocumented behavior, but I think things are working fine now.

vaughnbetz · 2020-09-17T01:34:05Z

Nice debugging; that's a subtle one.

probot-autolabeler bot added docs Documentation infra Project Infrastructure lang-cpp C/C++ code lang-perl Perl code lang-python Python code scripts Utility & Infrastructure scripts tests VTR Flow VTR Design Flow (scripts/benchmarks/architectures) labels Aug 27, 2020

shadtorrie mentioned this pull request Aug 27, 2020

Python requirements added #1510

Merged

7 tasks

shadtorrie changed the title ~~Vtr task and test rewrite~~ Vtr task and test script python rewrite Aug 27, 2020

HackerFoo self-requested a review August 27, 2020 20:19

probot-autolabeler bot added build Build system lang-make CMake/Make code lang-shell Shell scripts (bash etc.) Odin Odin II Logic Synthesis Tool: Unsorted item labels Aug 27, 2020

litghost added the kokoro:force-run label Aug 27, 2020

symbiflow-robot removed the kokoro:force-run label Aug 27, 2020

HackerFoo suggested changes Aug 27, 2020

View reviewed changes

shadtorrie requested a review from HackerFoo September 5, 2020 17:07

vaughnbetz added the kokoro:force-run label Sep 7, 2020

symbiflow-robot removed the kokoro:force-run label Sep 7, 2020

HackerFoo reviewed Sep 8, 2020

View reviewed changes

jgoeders added the kokoro:force-run label Sep 11, 2020

symbiflow-robot removed the kokoro:force-run label Sep 11, 2020

jgoeders added the kokoro:force-run label Sep 12, 2020

symbiflow-robot removed the kokoro:force-run label Sep 12, 2020

Converting task and test functionality to python

9313160

shadtorrie force-pushed the vtr_task_and_test_rewrite branch from 8a254de to 9313160 Compare September 13, 2020 02:07

cleaning up .travis.yml after requriements were added

4888a84

jgoeders added the kokoro:force-run label Sep 13, 2020

symbiflow-robot removed the kokoro:force-run label Sep 13, 2020

jgoeders added the kokoro:force-run label Sep 14, 2020

symbiflow-robot removed the kokoro:force-run label Sep 14, 2020

jgoeders added the kokoro:force-run label Sep 15, 2020

symbiflow-robot removed the kokoro:force-run label Sep 15, 2020

jgoeders added the kokoro:force-run label Sep 16, 2020

symbiflow-robot removed the kokoro:force-run label Sep 16, 2020

Task thread Pool now uses apply instead of starmap

8344ec4

jgoeders added the kokoro:force-run label Sep 16, 2020

symbiflow-robot removed the kokoro:force-run label Sep 16, 2020

jgoeders merged commit 8508bbe into verilog-to-routing:master Sep 17, 2020

jgoeders mentioned this pull request Sep 17, 2020

Remove old perl scripts #1551

Merged

7 tasks

		my $task_name = shift;
		(my $task_path = $task_name) =~ s/\s+$//;

Vtr task and test script python rewrite #1509

Vtr task and test script python rewrite #1509

Uh oh!

Conversation

shadtorrie commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

litghost commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shadtorrie commented Aug 27, 2020

Uh oh!

shadtorrie commented Aug 27, 2020

Uh oh!

shadtorrie commented Aug 27, 2020

Uh oh!

HackerFoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaughnbetz commented Sep 4, 2020

Uh oh!

shadtorrie commented Sep 6, 2020

Uh oh!

vaughnbetz commented Sep 7, 2020

Uh oh!

vaughnbetz commented Sep 7, 2020

Uh oh!

shadtorrie commented Sep 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shadtorrie commented Aug 27, 2020 •

edited

Loading

litghost commented Aug 27, 2020 •

edited

Loading

shadtorrie commented Sep 7, 2020 •

edited

Loading

vaughnbetz commented Sep 15, 2020 •

edited

Loading